Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not print message twice with ClickException #2330

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Do not print message twice with ClickException #2330

merged 3 commits into from
Apr 8, 2024

Conversation

thomasjpfan
Copy link
Member

Why are the changes needed?

When click.ClickException is raised, the message is already printed. For example, running this:

import click

from flytekit.clis.sdk_in_container.utils import ErrorHandlingCommand


@click.group("cli", invoke_without_command=True, cls=ErrorHandlingCommand)
@click.option(
    "-v",
    "--verbose",
    required=False,
    count=True,
    default=0,
    type=int,
)
def main(verbose):
    my_error_msg = "This is an error message"
    raise click.ClickException(my_error_msg)


if __name__ == "__main__":
    main()

On master I get the error message twice:

Screenshot 2024-04-04 at 5 16 05 PM

With this PR, the error message appears once:

Screenshot 2024-04-04 at 5 19 22 PM

@thomasjpfan
Do not print message twice
de50b0d 
Signed-off-by: Thomas J. Fan <[email protected]>
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Apr 4, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 4, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.57%. Comparing base (bf38b8e) to head (de50b0d).
Report is 1 commits behind head on master.

❗ Current head de50b0d differs from pull request most recent head 6be81b0. Consider uploading reports for the commit 6be81b0 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           master    #2330       +/-   ##
===========================================
- Coverage   83.04%   53.57%   -29.47%     
===========================================
  Files         324      180      -144     
  Lines       24861    18133     -6728     
  Branches     3547     3547               
===========================================
- Hits        20645     9715    -10930     
- Misses       3591     7991     +4400     
+ Partials      625      427      -198

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kumare3
kumare3 previously approved these changes Apr 5, 2024
View reviewed changes
@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Apr 5, 2024
@eapolinario eapolinario merged commit 8e916bc into flyteorg:master Apr 8, 2024
47 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
@thomasjpfan @eapolinario @fiedlerNr9
Do not print message twice with ClickException (#2330)
b722cfc 
* Do not print message twice

Signed-off-by: Thomas J. Fan <[email protected]>

* Remove check of string in output

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eapolinario eapolinario eapolinario approved these changes

@kumare3 kumare3 kumare3 left review comments

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@pingsutw pingsutw Awaiting requested review from pingsutw pingsutw is a code owner

@cosmicBboy cosmicBboy Awaiting requested review from cosmicBboy cosmicBboy is a code owner

@samhita-alla samhita-alla Awaiting requested review from samhita-alla samhita-alla is a code owner

Assignees
No one assigned
Labels
lgtm This PR has been approved by maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thomasjpfan @eapolinario @kumare3